-
Notifications
You must be signed in to change notification settings - Fork 950
Add support for MLDSA in hsmtool #29202
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
sw/host/hsmtool/src/util/signing.rs
Outdated
| // Data is a slice of plaintext: hash. | ||
| SignData::Slice(a, b) => Self::data_plain_text(&input[*a..*b]), | ||
| }, | ||
| KeyType::MlDsa => match self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does ML-DSA have a notion of input types categorized into signing domains like SLH-DSA does?
If no, then this is the correct way to handle the data preparation.
If yes, then we may need something a bit like the spx_prepare function below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes there is a "pure" and "prehash" version of MLDSA as well. I updated with mldsa_prepare below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we eliminate MlDsa from the prepare function since the mldsa operations should be using mldsa_prepare (and it should be an error to prepare when there is a more specific domain-enabled preparation function).
|
It would be nice to add some tests for MLDSA into //sw/host/hsmtool/tests. |
9147cb3 to
7be7c25
Compare
7be7c25 to
7ceb7c3
Compare
Yeah, I'll file an issue to add tests for MLDSA, currently softhsmv2 MLDSA is still in a PR softhsm/SoftHSMv2#809 (hopefully merged soon). I think it makes most sense to update the softhsmv2 dependency once MLDSA support is merged. I only need this change so i can test opentitan-provisioning locally against the softhsmv2 fork. |
| let val = map | ||
| .get(&AttributeType::Value) | ||
| .ok_or(anyhow!("Key does not contain a value"))?; | ||
| let key_value: Vec<u8> = val.try_into()?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What (if anything) does the PKCS#11 spec say about the encoding of the Value field?
I see that we're using this as-is later to save the key as either DER or PEM form. This is ok as long as the Value field is already DER encoded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah good catch this is wrong, I was just writing the raw keys to a file which is wrong.
I imported the ml_dsa rust library to perform the encoding to PEM/DER for export
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This encode/decode stuff maybe belongs in //sw/host/hsmtool/src/util/key/.... The existing code for ecdsa and rsa has TryFrom implementations that convert between the RustCrypto forms and AttributeMap. This is meant to hide the ugliness of the conversions from the code that implements the command hierarchy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved the encode/decode logic to a seperate file under //sw/host/hsmtool/src/util/key/mldsa.rs
sw/host/hsmtool/src/util/signing.rs
Outdated
| // Data is a slice of plaintext: hash. | ||
| SignData::Slice(a, b) => Self::data_plain_text(&input[*a..*b]), | ||
| }, | ||
| KeyType::MlDsa => match self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we eliminate MlDsa from the prepare function since the mldsa operations should be using mldsa_prepare (and it should be an error to prepare when there is a more specific domain-enabled preparation function).
a4d3e70 to
6e782dd
Compare
|
|
||
| #[derive(clap::Args, Debug, Serialize, Deserialize)] | ||
| pub struct Export { | ||
| #[arg(long)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can you provide doc comments for each parameter?
Updates the cryptoki crate to version 0.12.0 and cryptoki-sys to 0.5.0. Removes local patches that are now upstreamed and updates the library initialization code to match the new API. Signed-off-by: Willy Zhang <[email protected]>
Adds commands to generate, export, sign, and verify using the MLDSA algorithm (defaulting to ML-DSA-87). Includes support for exporting CSRs with the correct ML-DSA-87 OID. Signed-off-by: Willy Zhang <[email protected]>
Signed-off-by: Willy Zhang <[email protected]>
Signed-off-by: Willy Zhang <[email protected]>
Signed-off-by: Willy Zhang <[email protected]>
This updates the PKCS#11 constant generation script to support MLDSA aliases and adds a workaround for handling MLDSA ParameterSet attributes in the hsmtool library. It also adds necessary dependencies to the build file. Signed-off-by: Willy Zhang <[email protected]>
Signed-off-by: Willy Zhang <[email protected]>
Moves the MLDSA key encoding and decoding logic from the import/export commands into a dedicated utility module at `sw/host/hsmtool/src/util/key/mldsa.rs`. Signed-off-by: Willy Zhang <[email protected]>
Renames the 'mldsa generate' command to 'mldsa generate-keypair' for clarity. Adds comprehensive doc comments to all MLDSA command structures (export, export-csr, generate-keypair, import, sign, verify) to improve user guidance. Signed-off-by: Willy Zhang <[email protected]>
Signed-off-by: Willy Zhang <[email protected]>
1d35cc6 to
5c0000b
Compare
|
Rebased my PR onto origin/main to see if that resolves the presubmit failures |
Bumps up cryptoki dep version and adds MLDSA support. Tested against fork of softhsmv2 with MLDSA support on the opentitan-provisioning repo.